Problem: go-ethereum is not updated #1247
Conversation
WalkthroughThe changes revolve around updates to Ethereum-related modules, particularly involving gas calculations, EVM parameter key tables, and chain configuration. The updates include the removal of statefulness checks in precompile contracts, adjustments to gas requirements to align with actual consumption, and the introduction of new protocol buffer definitions for various EVM components. Additionally, there's a shift to accommodate the Shanghai upgrade in fee verification and intrinsic gas calculations. Changes
TipsChat with CodeRabbit Bot (
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1247 +/- ##
===========================================
+ Coverage 36.81% 48.13% +11.32%
===========================================
Files 115 74 -41
Lines 10287 6197 -4090
===========================================
- Hits 3787 2983 -804
+ Misses 6126 2896 -3230
+ Partials 374 318 -56
|
mmsqe
force-pushed
the
bump_v1.11.0
branch
2 times, most recently
from
4bf75fc to
dadd67b
Compare
mmsqe
requested review from
yihuang and
leejw51crypto
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- go.mod
- go.sum
- gomod2nix.toml
Files selected for processing (7)
- CHANGELOG.md (1 hunks)
- app/app.go (2 hunks)
- app/upgrades.go (2 hunks)
- x/cronos/keeper/grpc_query.go (2 hunks)
- x/cronos/keeper/precompiles/bank.go (1 hunks)
- x/cronos/keeper/precompiles/ica.go (1 hunks)
- x/cronos/keeper/precompiles/relayer.go (2 hunks)
Additional comments: 9
CHANGELOG.md (1)
- 13-17: The changelog correctly documents the significant updates made to the relayer precompile and the dependencies, including the go-ethereum version bump. This aligns with the pull request summary provided.
x/cronos/keeper/precompiles/bank.go (1)
- 85-90: The
IsStatefulfunction has been removed from theBankContractstruct. Ensure that this change does not affect any other part of the system that might rely on the statefulness information of the contract. If the function was removed because all contracts are now assumed to be stateful or stateless, this should be clearly documented to avoid confusion.x/cronos/keeper/precompiles/relayer.go (2)
69-75: The
core.IntrinsicGasfunction call has been updated to include additional boolean arguments. Verify that these arguments correctly represent the desired transaction type and that they align with the updated go-ethereum version's requirements for intrinsic gas calculation.90-90: The
RequiredGasmethod now correctly calculates the required gas by subtracting the intrinsic gas from the total. This change seems to be in line with the updated go-ethereum library's gas calculation requirements.app/app.go (1)
- 1154-1160: The change from
evmtypes.ParamKeyTable()tov0evmtypes.ParamKeyTable()is significant as it indicates a versioning or restructuring of the EVM module's parameter key table. Ensure that this change is consistent with the intended versioning strategy and that it does not introduce any compatibility issues with existing parameters or state.app/upgrades.go (2)
30-36: The import for
v0evmtypeshas been added, which suggests a new version or migration path for the EVM module types. Ensure that this new import is used consistently throughout the codebase and that any deprecated types are no longer in use.64-70: The
ParamKeyTablefor theevmtypes.ModuleNamehas been updated to usev0evmtypes.ParamKeyTable(). This change should be verified to ensure that it aligns with the intended migration strategy and that the new key table is compatible with existing parameters and their usage within the module.x/cronos/keeper/grpc_query.go (2)
67-73: The introduction of the
shanghaivariable is consistent with the summary provided. It's important to ensure that theIsShanghaifunction is correctly implemented and tested, as it now plays a critical role in fee verification.83-89: The
VerifyFeefunction signature has been updated to include theshanghaiparameter. This change should be cross-checked with theevmkeepermodule to ensure that the function definition is updated accordingly and that the new parameter is being used correctly within the function.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- app/app.go (1 hunks)
Additional comments: 2
CHANGELOG.md (1)
- 13-17: The changelog correctly reflects the changes made in pull requests Problem: require gas in relayer precompile is higher than consumed #1232 and Problem: go-ethereum is not updated #1247. The description of the adjustment to the required gas in the relayer precompile is clear, and the update of dependencies is accurately documented.
app/app.go (1)
- 1153-1159: The removal of
WithKeyTablemethod call for theevmtypes.ModuleNamesubspace in theinitParamsKeeperfunction is consistent with the change summary. However, ensure that the removal does not affect the parameter key table configuration for the EVM module and that the application still functions as expected without it.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -67,6 +67,7 @@ | |||
| blockHeight := big.NewInt(req.BlockNumber) | |||
| homestead := ethCfg.IsHomestead(blockHeight) | |||
| istanbul := ethCfg.IsIstanbul(blockHeight) | |||
| shanghai := ethCfg.IsShanghai(uint64(req.BlockTime.Unix())) | |||
Check failure
Code scanning / gosec
Potential integer overflow by integer type conversion Error
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (4)
- client/docs/swagger-ui/swagger.yaml
- go.mod
- go.sum
- gomod2nix.toml
Files selected for processing (17)
- third_party/proto/ethermint/evm/v1/access_tuple.proto (1 hunks)
- third_party/proto/ethermint/evm/v1/chain_config.proto (1 hunks)
- third_party/proto/ethermint/evm/v1/chain_config_v0.proto (1 hunks)
- third_party/proto/ethermint/evm/v1/genesis.proto (1 hunks)
- third_party/proto/ethermint/evm/v1/log.proto (1 hunks)
- third_party/proto/ethermint/evm/v1/params.proto (1 hunks)
- third_party/proto/ethermint/evm/v1/params_v0.proto (1 hunks)
- third_party/proto/ethermint/evm/v1/params_v4.proto (1 hunks)
- third_party/proto/ethermint/evm/v1/query.proto (2 hunks)
- third_party/proto/ethermint/evm/v1/state.proto (1 hunks)
- third_party/proto/ethermint/evm/v1/trace_config.proto (1 hunks)
- third_party/proto/ethermint/evm/v1/trace_config_v0.proto (1 hunks)
- third_party/proto/ethermint/evm/v1/transaction_logs.proto (1 hunks)
- third_party/proto/ethermint/evm/v1/tx.proto (2 hunks)
- third_party/proto/ethermint/evm/v1/tx_result.proto (1 hunks)
- x/cronos/keeper/grpc_query.go (2 hunks)
- x/cronos/keeper/precompiles/relayer.go (3 hunks)
Files skipped from review due to trivial changes (1)
- third_party/proto/ethermint/evm/v1/chain_config.proto
Additional comments: 21
third_party/proto/ethermint/evm/v1/genesis.proto (1)
- 1-8: The changes to the import statements and
go_packageoption are consistent with the summary provided. Ensure that the new imports are correctly referenced in the rest of the codebase and that the updatedgo_packagepath is valid and accessible.third_party/proto/ethermint/evm/v1/log.proto (1)
- 1-37:
The protobuf definition for theLogmessage is well-structured and includes all necessary fields with appropriate data types and JSON tags for Ethereum log events.third_party/proto/ethermint/evm/v1/transaction_logs.proto (1)
- 1-16:
The protobuf definition forTransactionLogsis well-defined and includes all necessary fields and imports as per the summary.third_party/proto/ethermint/evm/v1/state.proto (1)
- 1-12:
The protobuf definition for theStatemessage is correctly implemented.third_party/proto/ethermint/evm/v1/access_tuple.proto (1)
- 1-16:
The newAccessTuplemessage definition appears to be correctly implemented according to the proto3 specification and is consistent with the project's use of gogoproto extensions.third_party/proto/ethermint/evm/v1/params_v0.proto (1)
- 1-25:
The protobuf messageV0Paramsis correctly defined and follows the proto3 syntax and conventions. The fields are well-documented, and the use ofgogoprotooptions for custom names and tags is appropriate.third_party/proto/ethermint/evm/v1/trace_config.proto (1)
- 1-39:
The protobuf definition forTraceConfigcorrectly reflects the changes described in the summary, with appropriate deprecation and renaming of fields. The use of reserved fields and tags ensures backward compatibility and prevents conflicts with future changes.third_party/proto/ethermint/evm/v1/params.proto (1)
- 1-25:
The protobuf definition for
Paramsis correctly defined and includes all the necessary fields and options as per the summary provided. The use ofgogoprotooptions for custom names and YAML tags is appropriate, and the import statements are correctly specified.third_party/proto/ethermint/evm/v1/query.proto (2)
2-11:
The import changes align with the pull request summary, ensuring the protobuf file reflects the updated dependencies and new functionality.227-231:
The addition of theoverridesfield inEthCallRequestis consistent with the change summary, allowing for state overrides during EVM calls.third_party/proto/ethermint/evm/v1/params_v4.proto (1)
- 1-31:
The protobuf definitions for
V4ParamsandExtraEIPsare well-defined and follow the standard conventions for defining messages and fields in proto3. The use ofgogoprotooptions for custom names and YAML tags is appropriate and enhances the compatibility with Go code and YAML serialization.x/cronos/keeper/grpc_query.go (2)
67-72:
The use ofuint64(req.BlockTime.Unix())could lead to an integer overflow. Verify thatreq.BlockTime.Unix()will not exceed the maximum value foruint64and consider adding overflow checks if necessary.83-89:
The changes to theVerifyFeefunction call correctly include the newshanghaiparameter. Ensure that theVerifyFeefunction is updated accordingly to handle this new parameter.third_party/proto/ethermint/evm/v1/trace_config_v0.proto (1)
- 1-39: - Ensure that all deprecated fields are no longer used in the codebase and that any necessary migrations to the new fields have been completed.
- Verify that the JSON tags provided in the protobuf definitions match the expected JSON structure in the rest of the application.
- Confirm that the
V0ChainConfigimport is used correctly and that the corresponding message is compatible with theoverridesfield inV0TraceConfig.- Check that the
go_packageoption is correctly set to match the project's directory structure and naming conventions.- Review the use of custom JavaScript tracers and ensure that the
tracerfield is properly sanitized and handled to prevent any potential security issues.- Validate that the
timeout,reexec,limit, andtracer_json_configfields are being correctly parsed and used whereverV0TraceConfigis instantiated.third_party/proto/ethermint/evm/v1/tx.proto (2)
3-13: The imports for "ethermint/evm/v1/access_tuple.proto", "ethermint/evm/v1/log.proto", and "ethermint/evm/v1/params.proto" have been correctly added to support the new protobuf definitions for EVM-related messages. Ensure that these new files are present in the project and that their definitions are being used correctly in the codebase.
34-41: The field "from" in the
MsgEthereumTxmessage has been correctly changed from a string to bytes, and a deprecated string field "deprecated_from" has been added. Ensure that all parts of the codebase that interact with the "from" field are updated to handle bytes instead of a string, and that the presence of "deprecated_from" is accounted for in any legacy handling or migration logic.third_party/proto/ethermint/evm/v1/tx_result.proto (1)
- 1-28: - Ensure that the
TxResultmessage fields align with the Ethereum standards and the intended use within the Crypto.org Chain project.
- Verify that the
TransactionLogsmessage is correctly defined and imported, as it is a critical part of theTxResult.- Confirm that the
contract_addressfield should be a string and not an Ethereum address type, which is typically represented as bytes.- Check that the
bloomfield is being correctly used and interpreted in the codebase, as bloom filters have specific encoding and decoding requirements.- Ensure that the
retfield is being handled correctly, especially in cases where the execution result is large.- Verify that the
revertedfield is being set and checked correctly throughout the codebase, as it is crucial for error handling.- Confirm that the
gas_usedfield is being calculated and used correctly, as gas usage is a fundamental aspect of EVM execution and transaction cost.x/cronos/keeper/precompiles/relayer.go (4)
45-54: The
RelayerContractstruct has been updated with new boolean fields to represent the status of Ethereum hard forks. Ensure that these fields are properly set and used throughout the codebase to maintain the correct behavior based on the active hard forks.69-73: The
SetChainConfigmethod has been added to set the Ethereum hard fork status flags in theRelayerContract. Verify that this method is called with the correct values during the initialization or configuration of the contract to reflect the actual chain configuration.77-84: The
RequiredGasmethod now uses the new hard fork status flags to calculate the intrinsic gas. Ensure that the error handling forcore.IntrinsicGasis adequate and that it doesn't silently fail, potentially causing issues with gas calculation.99-100: The
RequiredGasmethod correctly calculates the total gas required minus the intrinsic gas. Ensure that the gas calculation aligns with the updated Ethereum gas pricing rules post-hard forks.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (4)
- go.mod
- go.sum
- gomod2nix.toml
- scripts/geth-genesis.json
Files selected for processing (4)
- integration_tests/network.py (1 hunks)
- nix/go-ethereum.nix (2 hunks)
- scripts/start-geth (2 hunks)
- x/cronos/keeper/precompiles/relayer.go (3 hunks)
Files skipped from review due to trivial changes (2)
- integration_tests/network.py
- scripts/start-geth
Additional comments: 4
nix/go-ethereum.nix (2)
13-24:
The version update and checksum changes are correct and align with the standard Nix package update process.42-47:
The summary mentions the removal of "cmd/puppeth" from the list of components, but it is not reflected in thesubPackageslist. Please verify if this is an oversight.x/cronos/keeper/precompiles/relayer.go (2)
45-54: The
RelayerContractstruct has been extended with new boolean fields to represent chain configurations. Ensure that these fields are properly documented and that their usage is consistent throughout the codebase.69-78: The
WithChainConfigmethod is introduced to set the chain configuration flags on theRelayerContract. Ensure that this method is used correctly wherever aRelayerContractinstance is created or modified to reflect the intended chain configuration.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- go.mod
- go.sum
- gomod2nix.toml
Files selected for processing (2)
- app/app.go (3 hunks)
- x/cronos/keeper/precompiles/relayer.go (4 hunks)
Additional comments: 2
app/app.go (1)
- 1158-1164: The
WithKeyTablemethod call has been removed from theparamsKeeper.Subspacefor theevmtypes.ModuleNameandfeemarkettypes.ModuleName. However, theWithKeyTablemethod is still being used for other modules. Ensure that this change is intentional and that the key table is not required for theevmtypes.ModuleNameandfeemarkettypes.ModuleNamesubspaces.x/cronos/keeper/precompiles/relayer.go (1)
- 46-66: The addition of the
isHomestead,isIstanbul, andisShanghaifields to theRelayerContractstruct and their initialization in theNewRelayerContractfunction are consistent with the summary and appear to be correctly implemented.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- go.mod
- go.sum
- gomod2nix.toml
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- app/app.go (3 hunks)
- x/cronos/keeper/precompiles/ica.go (1 hunks)
Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Additional comments: 2
app/app.go (2)
152-162: The summary states that the package
github.com/ethereum/go-ethereum/core/vmwas removed, but it is still present in the imports. Please verify if this package should indeed be removed or if the summary is incorrect.1160-1166: The
initParamsKeeperfunction has been correctly updated to no longer use theevmtypes.ParamKeyTable()for theevmtypes.ModuleNamesubspace, aligning with the summary of changes.
63bfea7
👮🏻👮🏻👮🏻 !!!! REFERENCE THE PROBLEM YOUR ARE SOLVING IN THE PR TITLE AND DESCRIBE YOUR SOLUTION HERE !!!! DO NOT FORGET !!!! 👮🏻👮🏻👮🏻
PR Checklist:
make)make test)go fmt)golangci-lint run)go list -json -m all | nancy sleuth)Thank you for your code, it's appreciated! :)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Style
Chores
Tests
Please note that internal code changes, filenames, and specific functions have been omitted for confidentiality.